Endpoint for pretty print migration plan #3137
Conversation
Shubham8287
force-pushed
the
shub/pretty-endpint
branch
from
4fca9d9 to
68d0045
Compare
| } | ||
|
|
||
| impl MigrationToken { | ||
| pub fn hash(&self) -> spacetimedb_lib::Hash { |
There was a problem hiding this comment.
I have not included any secret in token generation which means, anyone with enough intent could recreate the token generation logic and bypass it. Considering this token act as UX safegaurd and not for security. I feel, this acceptable and we don't have to worry about managing a secret.
There was a problem hiding this comment.
I agree, but please add a comment to this effect in the code.
|
CI is breaking because of base branch. |
| @@ -835,6 +962,7 @@ where | |||
|
|
|||
| axum::Router::new() | |||
| .route("/", self.root_post) | |||
| .route("/print-plan/:name_or_identity", self.print_migration_plan) | |||
There was a problem hiding this comment.
I have used "print-plan" as endpoint instead of "pre-publish" as former sounds more clear.
There was a problem hiding this comment.
I disagree. pre-publish clearly relates this operation to publishing, whereas print-plan could do pretty much anything.
There was a problem hiding this comment.
Most notably, I would confuse print-plan with displaying a query plan for a given SQL query.
| @@ -469,6 +474,9 @@ pub struct PublishDatabaseQueryParams { | |||
| #[serde(default)] | |||
| clear: bool, | |||
| num_replicas: Option<usize>, | |||
| // `Hash` of `MigrationToken` to be checked if `MigrationPolicy::BreakClients` is set. | |||
There was a problem hiding this comment.
| // `Hash` of `MigrationToken` to be checked if `MigrationPolicy::BreakClients` is set. | |
| /// `Hash` of `MigrationToken` to be checked if `MigrationPolicy::BreakClients` is set. | |
| /// | |
| /// Users obtain such a hash from [`print_migration_plan`] | |
| /// via the `/database/:name_or_identity/pre-publish POST` route. | |
| /// This is a safeguard to require explicit approval for updates which will break clients. | |
| @@ -835,6 +962,7 @@ where | |||
|
|
|||
| axum::Router::new() | |||
| .route("/", self.root_post) | |||
| .route("/print-plan/:name_or_identity", self.print_migration_plan) | |||
There was a problem hiding this comment.
Most notably, I would confuse print-plan with displaying a query plan for a given SQL query.
| @@ -469,6 +474,9 @@ pub struct PublishDatabaseQueryParams { | |||
| #[serde(default)] | |||
| clear: bool, | |||
| num_replicas: Option<usize>, | |||
| // `Hash` of `MigrationToken` to be checked if `MigrationPolicy::BreakClients` is set. | |||
| token: Option<spacetimedb_lib::Hash>, | |||
| policy: Option<MigrationPolicy>, | |||
There was a problem hiding this comment.
I think it would be clearer to default this to Compatible rather than wrapping it in Option.
|
|
||
| #[derive(serde::Deserialize)] | ||
| pub struct PrintPlanQueryParams { | ||
| style: Option<PrettyPrintStyle>, |
There was a problem hiding this comment.
Again, defaulting would be preferred over Option.
|
|
||
| if database.owner_identity != auth.identity { | ||
| return Err(( | ||
| StatusCode::BAD_REQUEST, |
There was a problem hiding this comment.
| StatusCode::BAD_REQUEST, | |
| StatusCode::UNAUTHORIZED, |
| #[derive(Debug, Clone, PartialEq, Eq)] | ||
| pub enum MigrationPolicy { | ||
| Compatible, | ||
| BreakClients(spacetimedb_lib::Hash), |
There was a problem hiding this comment.
Please add doc comment describing this value and where to obtain it, like in the client-api crate.
| } | ||
|
|
||
| impl MigrationToken { | ||
| pub fn hash(&self) -> spacetimedb_lib::Hash { |
There was a problem hiding this comment.
I agree, but please add a comment to this effect in the code.
Description of Changes
Adds endpoint for for pretty printing migration plan.
It also changes current
publishendpoint to optionally provideMigrationTokenandMigrationPolicyto allow migration with breaking clients.API and ABI breaking changes
Backward compatible change to existing API and new Api
Expected complexity level and risk
2
Testing
publishendpoint.